Repository navigation
refactor(accounts)!: unify frame account and payer constructors - #55
nikhilkumar1612 wants to merge 3 commits into
Conversation
mariano-aguero
left a comment
There was a problem hiding this comment.
Thanks for unifying these constructors, the single entry point makes the account API much easier to follow.
|
thanks for the review @mariano-aguero will take a look at them in a while |
BREAKING CHANGE: Specialized EOA, P-256, and EOA payer constructors are replaced by toFrameAccount and toFramePayerAccount.
mariano-aguero
left a comment
There was a problem hiding this comment.
Thanks for the quick and thorough follow-up, nikhilkumar1612, the earlier points are all addressed and the new tests read really well.
| const signFrameTransaction = | ||
| parameters.signFrameTransaction ?? | ||
| (async (transaction: FrameTransaction) => { | ||
| await assertPayFrame(transaction) |
There was a problem hiding this comment.
The default payer signs any transaction that has a payment-only VERIFY frame targeting it, including one whose sender is the payer itself, and the test at test/accounts.test.ts:200 now pins that case as supported. When sender and payer are the same address, the payer entry { scheme: 1, signer: payer, msg: '0x' } is a full sig-hash signature whose resolved signer is also the sender, so a sponsor that signs transactions built by someone else could end up approving SENDER frames that execute as the payer (a transfer of its own balance, for instance), as long as the sender's default-code VERIFY accepts any entry that resolves to it. A self-paying EOA does not need a pay frame, since self_verify with scope 0x3 already covers it, so I would suggest having the default policy refuse isAddressEqual(transaction.sender, address) here and in getSignatureEntries, turning the test at line 200 into a rejection test, and adding a line to DESIGN.md saying the default payer applies no other policy (no cap on maxCost or on frame contents), so anyone running it as a sponsor knows to override signFrameTransaction.
| frame.mode === 1 && | ||
| frame.flags === 0x1 && |
There was a problem hiding this comment.
No test exercises these two conditions: every test that reaches the payer sets the targeted frame to mode 1 with flags 0x1, and the negative test at test/accounts.test.ts:228 fails only because the first frame of GOLDEN_TX has a null target. Deleting frame.mode === 1 or frame.flags === 0x1 keeps the suite green, and either change would let the default payer sign a transaction where the frame naming it is a SENDER frame or a VERIFY frame with scope 0x3, which is what this guard is there to prevent. Two small negative cases next to the existing one, a frame targeting the payer with flags 0x3 and another with mode 2, would pin both.
| await assertPayFrame(transaction) | ||
|
|
||
| let prepared = transaction | ||
| let signatureIndex = findOwnerSignatureIndex(prepared, owner, 'last', { |
There was a problem hiding this comment.
The 'last' direction here and 'first' in toFrameAccount.ts:190 are what keep sender and payer on separate entries when one key controls both, for example a delegated sender with explicit signer K and a payer owned by K, which gives [{1, K}, {1, K}]. Both entries hash the same, so if either direction were flipped both accounts would sign the same index and leave the other one as 0x, and no test covers that layout: the sender == payer test at test/accounts.test.ts:200 uses [null, K], where explicitSigner already rules out index 0, and test/signFrameTransaction.test.ts uses distinct owners. A case that runs signFrameTransaction(account, tx, { payer }) with a shared owner and asserts both indices recover to K would make the direction load-bearing in the suite.
| parameters.getSignatureEntries ?? | ||
| (async (transaction: FrameTransaction) => { | ||
| await assertPayFrame(transaction) | ||
| return [ownerSignatureEntry(owner, owner.address)] |
There was a problem hiding this comment.
For a P-256 owner this default produces a scheme-2 entry with an explicit signer, but the only P-256 payer test (test/accounts.test.ts:286) overrides getSignatureEntries and signFrameTransaction and calls signFrameSignature(tx, 1, owner) itself, so it checks its own callbacks rather than the defaults the PR description lists as covered. Breaking the scheme, the explicit signer, or the explicitSigner lookup for scheme 2 would all go unnoticed. Building toFramePayerAccount({ owner: p256Owner, address: OTHER }) with no overrides, a pay frame targeting OTHER and signatures: [], then asserting the entry is { scheme: 2, signer: owner.address } and that it verifies with P256.verify, would cover the real path.
| const code = (await client.request({ | ||
| method: 'eth_getCode', | ||
| params: [address, 'latest'], | ||
| })) as Hex | ||
| const expected = `0xef0100${parameters.delegateAddress.slice(2)}`.toLowerCase() | ||
| if (code.toLowerCase() !== expected) |
There was a problem hiding this comment.
Thanks for adding this check, it is what gives delegateAddress its meaning. Right now, though, both mocks that serve eth_getCode (test/p256.test.ts:159 and test/accounts.test.ts:448) return the delegation code whatever the params are, and the negative test returns 0x. Querying owner.address instead of address, or relaxing the comparison to something like code.endsWith(delegate), would pass every test. Recording the request in the P-256 test and asserting params: [DELEGATED_EOA, 'latest'], plus a negative case with code that ends in the delegate address but lacks the 0xef0100 prefix, would pin both halves.
| (async () => { | ||
| const useImplicitSigner = | ||
| parameters.delegateAddress === undefined && | ||
| ownerSignatureScheme(owner) === 1 && |
There was a problem hiding this comment.
This condition decides whether a P-256 owner whose identity equals the sender gets the explicit signer that DESIGN.md requires for P256 entries, but no test reaches it: the P-256 tests either pass a distinct address or bring their own entry (test/p256.test.ts:217). Removing ownerSignatureScheme(owner) === 1 would emit signer: null for that owner, the lookup would still match because a null signer resolves to the sender, and nothing would fail. Starting the test at test/p256.test.ts:217 from signatures: [] and asserting signatures[0].signer === signer.address would cover it.
| directly, normalizes `s` low, and encodes `r || s || qx || qy`. Passing `address` separately | ||
| represents a delegated EOA controlled by a different signer. When `delegateAddress` is | ||
| provided, construction verifies that `eth_getCode(address)` is exactly | ||
| `0xef0100 || delegateAddress`. Account-specific validation or signature policies |
There was a problem hiding this comment.
This paragraph describes delegateAddress only as a construction-time eth_getCode check, but toFrameAccount.ts:174 also uses it to switch a secp256k1 owner whose address equals the sender from signer: null to an explicit signer, which changes the entry bytes, the sig-hash and the billed calldata. The test at test/accounts.test.ts:442 shows the switch is intended, so since DESIGN.md is the binding reference, it would help to add a sentence here saying a delegated sender always gets explicit signer metadata.
| const address = await (parameters.getAddress?.() ?? | ||
| Promise.resolve(unresolvedAddress!)) |
There was a problem hiding this comment.
When a caller passes both address and getAddress, getAddress wins and address is dropped silently, while the JSDoc on address only says it defaults to owner.address. Now that getAddress runs exactly once at construction it is equivalent to address: await fn(), so either documenting the precedence or rejecting both at once (as the ownerless parameter types already do with their union) would avoid a quiet mismatch. The same applies to toFramePayerAccount.ts:83.
| const expected = `0xef0100${parameters.delegateAddress.slice(2)}`.toLowerCase() | ||
| if (code.toLowerCase() !== expected) | ||
| throw new FrameEncodeError( | ||
| `frame account ${address} delegates with ${code}, expected ${expected}`, |
There was a problem hiding this comment.
If address turns out to be a regular contract rather than a delegated EOA, eth_getCode returns its whole runtime code, up to about 48 KB of hex, and all of it ends up in this error message and in whatever logs it reaches. A delegation designator is exactly 48 characters as hex, so truncating anything longer keeps the useful case readable.
| `frame account ${address} delegates with ${code}, expected ${expected}`, | |
| `frame account ${address} delegates with ${code.length > 48 ? `${code.slice(0, 48)}...` : code}, expected ${expected}`, |
| if (code === undefined || !code.toLowerCase().startsWith('0xef0100')) | ||
| if (code === undefined || !/^0xef0100[0-9a-fA-F]{40}$/.test(code)) | ||
| throw new Error(`sender ${sender} does not have an EIP-7702 delegation`) | ||
| const delegateAddress = getAddress(`0x${code.slice(8)}`) |
There was a problem hiding this comment.
Since delegateAddress is derived from the same getCode result that toFrameAccount fetches again, the new constructor check can never fail in this script, and it adds a second eth_getCode round trip. As this script is the reference for the delegated P-256 flow, reading the expected implementation from an env variable (next to P256_SIGNER_IDENTITY) and dropping the local regex check would show the parameter doing its job.
Reject ambiguous addresses and unsafe self-paying payer defaults, strengthen delegation validation, and add regression coverage for P-256 and shared-owner signing
|
thanks for checking again @mariano-aguero fixes are in place, can you take a look again 🙏 |
What this changes
Replaces the specialized
toEoaFrameAccount,toP256FrameAccount, andtoEoaFramePayerAccountconstructors with the unifiedtoFrameAccountandtoFramePayerAccountAPIs.The unified constructors infer defaults from the supplied owner while allowing account-specific behavior to be overridden.
Why
The specialized constructors duplicated address resolution, nonce handling, signature-entry construction, and signing behavior.
A unified API gives applications one consistent integration point for:
privateKeyToAccountThis also makes it possible to add future authorization mechanisms without introducing another top-level constructor for every account type.
How the new behaviour is pinned
Which oracle covers this change, and why it could actually fail:
frame_tx_wire_tests.rsmaxCost, or thegasUseddecomposition)The account tests independently recover generated secp256k1 signatures and verify P-256 signatures. Existing wire-format, captured-chain, signature, gas, and receipt-oracle tests continue to pass unchanged.
Load-bearing rules
Did this touch anything under "Rules that are not style preferences" in
CONTRIBUTING.md, such as a derived gas constant, a golden vector, a captured fixture, the module import order, orencodeFrameTxvalidating?Checks
Only tick what you ran.
bun run testbun run typecheckbun run build && bunx @arethetypeswrong/cli --pack .(the build passed;@arethetypeswrong/clihas not been run)bun run test:live(optional, hits the public endpoint, not a gate)Additional verification:
scripts/temp.ts.Release impact
The commit history sets the version, so say which this is:
docs,chore,test,ci,refactor,style,perf,build)fix)feat)feat!or aBREAKING CHANGE:footer)This removes the following public exports:
toEoaFrameAccounttoP256FrameAccounttoEoaFramePayerAccountMigration examples: